[Testing] Fix data race in cluster-prefix RecordCache - #8627
Merged
Conversation
janezpodhostnik
changed the title
Contributor
📝 WalkthroughWalkthroughThe changes add vote consumer transition coverage, resource-limit override tests, locked cache gauge capture, and concurrent cluster-prefixed message validation coverage. ChangesVote consumer transition testing
Resource limit override testing
Inspector cache and validation concurrency
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
janezpodhostnik
force-pushed
the
janez/flaky-test-followup-coverage
branch
from
July 29, 2026 14:42
8aac74a to
1f7def0
Compare
zhangchiqing
approved these changes
Jul 30, 2026
Kay-Zee
approved these changes
Jul 31, 2026
janezpodhostnik
enabled auto-merge
August 3, 2026 14:42
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@consensus/hotstuff/votecollector/statemachine_test.go`:
- Around line 496-521: Update prepareMockedProcessor to require
Process(proposerVote) exactly expectedProposerVoteProcessings times when that
count is greater than zero, instead of leaving the expectation optional;
preserve the existing optional behavior when the count is zero so proposer-vote
replay is verified in the affected tests.
In
`@network/p2p/inspector/validation/control_message_validation_inspector_test.go`:
- Around line 1472-1479: Update the concurrent notification-count assertions in
the test around the cluster-prefix tracker threshold scenario to require exactly
5 notifications for 15 inspections with a hard threshold of 10, rather than
allowing any count up to total. Apply the same exact-count assertion to the
additional assertion near the later referenced block, preserving the existing
concurrency setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 905262c5-4185-43a8-ac16-cbc325508e7b
📒 Files selected for processing (4)
consensus/hotstuff/votecollector/statemachine_test.gonetwork/p2p/builder/libp2pscaler_test.gonetwork/p2p/inspector/internal/cache/cache.gonetwork/p2p/inspector/validation/control_message_validation_inspector_test.go
Comment on lines
+496
to
+521
| processor := s.prepareMockedProcessor(proposal, 1) | ||
| proposerVote, err := proposal.ProposerVote() | ||
| require.NoError(s.T(), err) | ||
|
|
||
| actualVotes := make([]*model.Vote, 0) | ||
| s.collector.RegisterVoteConsumer(func(vote *model.Vote) { | ||
| actualVotes = append(actualVotes, vote) | ||
| }) | ||
|
|
||
| expectedVotes := make([]*model.Vote, 0) | ||
| // Add votes while the collector is in the caching state. Each vote emits `OnVoteProcessed` | ||
| // twice: once when cached, and once when replayed into the verifying processor during the | ||
| // transition below (consistent with `TestProcessBlock_ProcessingOfCachedVotes`). | ||
| for i := 0; i < votes; i++ { | ||
| vote := unittest.VoteForBlockFixture(block) | ||
| s.notifier.On("OnVoteProcessed", vote).Twice() | ||
| processor.On("Process", vote).Return(nil).Once() | ||
| require.NoError(s.T(), s.collector.AddVote(vote)) | ||
| expectedVotes = append(expectedVotes, vote) | ||
| } | ||
|
|
||
| // `ProcessBlock` caches the proposer's vote (delivering it to the consumer) and transitions | ||
| // the collector to the verifying state | ||
| require.NoError(s.T(), s.collector.ProcessBlock(proposal)) | ||
| require.Equal(s.T(), hotstuff.VoteCollectorStatusVerifying, s.collector.Status()) | ||
| expectedVotes = append(expectedVotes, proposerVote) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline consensus/hotstuff/votecollector/statemachine_test.go --items all --match prepareMockedProcessor
rg -n -C 5 'prepareMockedProcessor\(' consensus/hotstuff/votecollector/statemachine_test.goRepository: onflow/flow-go
Length of output: 9038
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '70,100p' consensus/hotstuff/votecollector/statemachine_test.go
sed -n '220,256p' consensus/hotstuff/votecollector/statemachine_test.go
sed -n '318,331p' consensus/hotstuff/votecollector/statemachine_test.go
python3 - <<'PY'
from pathlib import Path
import re
p = Path('consensus/hotstuff/votecollector/statemachine_test.go')
src = p.read_text()
m = re.search(
r'func \(s \*StateMachineTestSuite\) prepareMockedProcessor\(proposal \*model\.SignedProposal, expectedProposerVoteProcessings int\) \*mocks\.VerifyingVoteProcessor \{(?P<body>.*?)\n\}',
src,
flags=re.S
)
if not m:
raise SystemExit('prepareMockedProcessor not found')
body = m.group('body')
print(body)
print('--- Process proposer vote occurrences ---')
for name in ['Process', 'OnVoteProcessed']:
print(name, body.count(name), 'found')
print(re.findall(r'(processor\.On\("(?:Process|OnVoteProcessed)"[^)]+\))', body))
PYRepository: onflow/flow-go
Length of output: 5704
Require processing of the proposer vote.
prepareMockedProcessor sets Process(proposerVote) to .Maybe(), while expectedProposerVoteProcessings only controls OnVoteProcessed. This makes the cached proposer-vote replay assertions in tests like TestProcessBlock_ProcessingOfCachedVotes and TestProcessBlock_ByzantineLeaderSpamming_ProposalAfterVote pass without a required Process(proposerVote) expectation. Make the helper also set .Times(expectedProposerVoteProcessings) for Process(proposerVote) when the count is greater than zero.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@consensus/hotstuff/votecollector/statemachine_test.go` around lines 496 -
521, Update prepareMockedProcessor to require Process(proposerVote) exactly
expectedProposerVoteProcessings times when that count is greater than zero,
instead of leaving the expectation optional; preserve the existing optional
behavior when the count is zero so proposer-vote replay is verified in the
affected tests.
Comment on lines
+1472
to
+1479
| params.Config.ClusterPrefixedMessage.HardThreshold = 10 | ||
| params.Config.GraftPrune.InvalidTopicIdThreshold = 0 | ||
| // This test deliberately keeps the default (multi-)worker count and documents the | ||
| // concurrency-tolerant contract: the cluster-prefix tracker increment and the subsequent | ||
| // hard-threshold check are not atomic, so each of the `total` concurrent inspections may | ||
| // observe the tracker above the hard threshold and disseminate a notification. At least | ||
| // one notification is guaranteed (the globally last inspection observes all increments), | ||
| // and at most one notification per inspected message can be disseminated. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert the exact threshold-exceeded notification count.
The locked gauge capture gives each inspection its own post-increment gauge. With a threshold of 10 and 15 inspections, only inspections 11 through 15 can notify. The current upper bound of total permits a regression that notifies for messages at or below the threshold.
Proposed fix
- const total = 15
+ const (
+ total = 15
+ hardThreshold = 10
+ )
...
- params.Config.ClusterPrefixedMessage.HardThreshold = 10
+ params.Config.ClusterPrefixedMessage.HardThreshold = hardThreshold
...
- count := notificationCount.Load()
- require.GreaterOrEqual(t, count, int64(1), "at least one inspection past the hard threshold must disseminate a notification")
- require.LessOrEqual(t, count, int64(total), "at most one notification per inspected message may be disseminated")
+ require.Equal(t, int64(total-hardThreshold), notificationCount.Load())Also applies to: 1507-1509
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@network/p2p/inspector/validation/control_message_validation_inspector_test.go`
around lines 1472 - 1479, Update the concurrent notification-count assertions in
the test around the cluster-prefix tracker threshold scenario to require exactly
5 notifications for 15 inspections with a hard threshold of 10, rather than
allowing any count up to total. Apply the same exact-count assertion to the
additional assertion near the later referenced block, preserving the existing
concurrency setup.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #8626, which fixed flaky tests by (among other things) weakening some
assertions. This PR restores that coverage — and fixes a production data race the new
coverage immediately exposed.
Data race fix in
RecordCache(production code)network/p2p/inspector/internal/cache/cache.go:ReceivedClusterPrefixedMessageandGetWithInitread.Gaugefrom the record pointer returned byAdjustWithInitafterthe backend lock is released. Records are mutated in place, so this read races with
concurrent adjustments of the same record. The inspector runs 5 inspection workers by
default, so this race is live on production nodes; #8626 pinning the cluster-prefix test
to a single worker had hidden it from the test suite.
Fix: capture the gauge inside the locked adjust closure instead of dereferencing the
returned pointer.
New/extended tests
network/p2p/inspector/validation: new multi-worker companion subtest ofTestNewControlMsgValidationInspector_validateClusterPrefixedTopic— with the defaultworker count, between 1 and one-per-message notifications may be disseminated past the
hard threshold (the tracker increment and threshold check are not atomic). This is the
test that surfaced the race above.
consensus/hotstuff/votecollector: newTestRegisterVoteConsumer_CachingToVerifyingTransition— a vote consumer keepsreceiving votes, in arrival order, across the caching→verifying transition.
network/p2p/builder: negative-value cases inTestApplyResourceLimitOverrideand newTestBuildLibp2pResourceManagerLimits_NonPositiveOverridesKeepDefaults— non-positiveoverrides keep the scaled defaults and are never passed through to rcmgr, where negative
values carry special meaning (-1 unlimited, -2 block-all).
All touched packages pass
go test -racelocally (repeated runs).Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
Bug Fixes
Tests